Conversation
|
I created a new PR on your behalf to resolve conflicts. You still need to add readme and fix code |
63586b4 to
6f86142
Compare
| e.printStackTrace(); | ||
| } | ||
| // Корректно ли здесь возвращать null или нет? | ||
| return null; |
There was a problem hiding this comment.
In this case upon returning null and assigning it to inputFull in line 23 you risk getting NullPointerExc in line 76 since you haven't checked s for null.
There was a problem hiding this comment.
It'll be better to return an empty string. In that way when you will parse it further, in the parseInput() method, you get an exception you handle. Returning null it's not a good manner.
| int x = (int) (Math.random() * 26 + 65); | ||
| table[i][j] = (char) x; | ||
| if (x % 2 == 0) { | ||
| strategyEven = strategyEven + " " + table[i][j] + ","; |
There was a problem hiding this comment.
Because of frequent concatenation it's better to use StringBuilder type for this variables (there would be no new object creation for each concatenation).
Also there is no need to announce this variables as global because you pass reference each time you call methods to operate on them.
There was a problem hiding this comment.
Also, using StringBuilder allows you to remove the deleteLastCommaInStrategy() method because StringBuilder has a method deleteCharAt(int ind).
| } | ||
|
|
||
| if (strategy.equals("odd")) { |
There was a problem hiding this comment.
May just replace with:
} else {
System.out.print(strategyOdd);
}
and do not check condition and call extra method.
There was a problem hiding this comment.
Try to use "text".equals(string) instead of string.equals("text"). Because the second form can throw NPE in some cases. Not in your case, but it's better to get used right form.
About whole code: I'm not sure about empty lines between strings of code but spaces and code style have been written correctly according to the code convention.
There was a problem hiding this comment.
Thank you guys for your remarks. Fixed it!
| if (str.endsWith(",")) { | ||
| return str.substring(0, str.length() - 1); | ||
| } else { | ||
| return str; |
There was a problem hiding this comment.
If you use StringBuilder instead there would be just
str.deleteCharAt(str.length() - 1);
and no need to return anything.
Then it's not necessary to add special method for this. Just do it in one line after filling string of letters.
# Conflicts: # README.md # src/main/java/homework_1/Main.java
…ure/TomShandrikov # Conflicts: # README.md
|
|
||
| import java.util.Scanner; | ||
|
|
||
| public class Game extends Thread { |
There was a problem hiding this comment.
Opt: Also, please introduce automatic ship placement
There was a problem hiding this comment.
Good job! Nice interface, well-chosen abstractions, architecture also looks fine. Good skills in Java Core and Java libraries. Special thanks for the description file and png, it helps.
To make better(opt): introduce automatic ship placement and PvE gameplay.
Approved!
There was a problem hiding this comment.
Automatic ship placement: Done!
There was a problem hiding this comment.
Player vs Computer(easy) mode: Done
| public void run() { | ||
| setUpNames(); | ||
| setUpShips(); | ||
| startGame(); |
There was a problem hiding this comment.
it's more like play();
| } | ||
| } | ||
|
|
||
| // You can uncomment this block of code instead above for() (lines: 28-42) to place only 2 ships for every player. |
There was a problem hiding this comment.
That's a code smell, please remove
| public class MyShots extends Board { | ||
| } |
There was a problem hiding this comment.
I don't understand the purpose of this class
| public abstract class InputReader { | ||
| protected Scanner scanner; | ||
|
|
||
| public String readLine() { | ||
| return scanner.nextLine(); | ||
| } | ||
|
|
||
| public Scanner getScanner() { | ||
| return scanner; | ||
| } | ||
| } |
There was a problem hiding this comment.
The purpose of this class in also inclear
There was a problem hiding this comment.
This is the only question I have: how could I avoid this abstraction and use tests in this way:
String input = "a1 h\n" +
"a3 h\n" +
...
"\n";
setInput(input);
Because if I have Scanner.in inside class I am testing I have the same exception all the time: no line found. So I solved it this way. Solution for tests...
| Parameterized constructor must perform Deep Copy. | ||
| */ | ||
|
|
||
| public final class ImmutableClass { |
There was a problem hiding this comment.
The task description is not followed:
несколько конструкторов
метод, который возвращает модифицированный (новый) обьект
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(TYPE) | ||
| @Inherited | ||
| public @interface ClassCoffeeAnnotation { |
There was a problem hiding this comment.
Nice, good examples!
| import java.nio.file.Paths; | ||
| import java.util.Scanner; | ||
|
|
||
| public class CustomFileReader { |
|
|
||
| //Basic Singleton | ||
|
|
||
| public class Singleton { |
| package homework_7.kittenToCatFunction; | ||
|
|
||
| @FunctionalInterface | ||
| public interface KittenToCatFunction<C extends Cat, K extends Kitten>{ |
There was a problem hiding this comment.
Why do you need generics if you don't use them?
| public static void main(String[] args) { | ||
|
|
||
| Kitten smallCat = new Kitten(1, "Push"); | ||
| KittenToCatFunction function = x -> new Cat(x.getAge() + 2, x.getName(), smallCat); |
There was a problem hiding this comment.
Good example with Cat having Kitten as a field. I just don't understand why it always have smallCat, but not the provided one...
| private String filename = "src/main/resources/custom_file_reader/file.txt"; | ||
|
|
||
| @Test | ||
| void givenTextFile_whenRun1_thenEquals() throws IOException { |
NikolaevArtem
left a comment
There was a problem hiding this comment.
Well-done! I'll approve your homework now, but you'll need to fix HW_3. In general - pretty good! I like that for many homework you experimented and did smth extra, not just the min requirement
Approved!


No description provided.